Support hot reload over websocket #2616
Conversation
|
|
||
| /// Pending hot reload requests waiting for a response from the client. | ||
| /// Keyed by the request ID. | ||
| final _pendingHotReloads = <String, Completer<HotReloadResponse>>{}; |
There was a problem hiding this comment.
The way the hot reload works in the client is it looks at the hotReloadSourcesUri which contains all the changed files and their libraries and then does an XHR. If whatever app that calls DWDS sends requests too quickly, it may be a possible that a hot reload request reads a version of the file that is no longer the correct version.
For example, the app sends a request and then sets the data in that URI. The app sends a second request with new data in that URI before the first request finishes. Then the first request is reading the new data instead.
There was a problem hiding this comment.
I see. This makes sense. If I add a request/response mechanism for fetchLibrariesForHotReload, similar to how I handle HotReloadRequest with a Completer, would that help resolve the possible race condition you described? Essentially by sending a FetchLibrariesForHotReloadRequest and waiting for its response (using a Completer), we can ensure that the client fetches the correct set of changed libraries before proceeding to the hot reload step. What do you think about this approach?
There was a problem hiding this comment.
I think we want to have the completer wait for both fetchLibrariesForHotReload and hotReload. The former may return the right paths now, but those paths may be replaced with new compiled files before hotReload gets to execute if we don't wait for hotReload as well.
But I believe that should work. This more or less "blocks" the request queue until the previous request is fully finished.
| @@ -366,6 +370,49 @@ Future<bool> _authenticateUser(String authUrl) async { | |||
| return responseText.contains('Dart Debug Authentication Success!'); | |||
| } | |||
|
|
|||
| // handle hot reload request/response logic. | |||
There was a problem hiding this comment.
nit: Remove this comment or add more detail. Function name gives this same info.
| final requestId = event.id; | ||
| try { | ||
| // Execute the hot reload. | ||
| await manager.fetchLibrariesForHotReload(hotReloadSourcesPath); |
There was a problem hiding this comment.
Based on the comments above it seems like these both need to happen before we 'ack' the hot reload request. I'm not clear on a couple things:
- Why do these need to be 2 separate functions if we need to make sure they're called together anyway?
- Once the future from
fetchLibrariesForHotReloadcompletes the files are loaded into the page and we have an ID associated with the request so we can avoid mixing up files. Why do we need to wait for the client to finish applying the hot reload patch (i.e. callinghotReload) to 'ack' the server and unblock it to process further requests?
cc @srujzs
reloadSourcesinChromeProxyServiceandDevHandlercan now handle hot reload requests and responses over WebSockets (protected by bool flaguseWebSocket).Related to #2605